-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 56: Add pathfinder init #117
Conversation
I took a step back and rather than trying to implement a full loop of model definition -> init -> NUTS, I've refocussed on having a robust initialisation method. So now I implement an exported
|
Co-authored-by: Sam Abbott <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much nicer I think. Some safety and quality of life comments + the obvious big merge issue.
Once this has stabilised and we have taken it out for a spin it would be interesting to float this wrapper to |
…thout-renewal into add-pathfinder-init
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
==========================================
- Coverage 97.76% 97.61% -0.15%
==========================================
Files 6 7 +1
Lines 134 168 +34
==========================================
+ Hits 131 164 +33
- Misses 3 4 +1 ☔ View full report in Codecov by Sentry. |
I've increased the testing for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed face to face reordering the inputs to match multipathfinder
but think that has yet to happen? Once that is done and we have a new issue for falling back on more functionality from multipathfinder
I think this is good to go. It also sounds like we have some confusion about the aim with the docstrings but again perhaps that needs a new issue to discuss..
I've changed the input to match |
After a quick scoping, I don't have immediate plans to use more |
Why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I thought that the |
This is a new draft PR aimed at addressing #56 .
The main innovation for this PR occurs here:
Rt-without-renewal/EpiAware/docs/src/examples/getting_started_pf.jl
Lines 245 to 291 in ac788d0
The aim is to ultimately have
_epi_aware
as an _internal_method as part of a wider workflow. But for this PR it is getting this functionality available.The current first pass behaviour is for
_epi_aware
to take in the information required to act onmake_epi_aware
; then domultipathfinder
and pass the initial parameters tosample
(as per this workflow).Close #56